Skip to content

ms defender: do not cache parsed findings#12493

Merged
valentijnscholten merged 2 commits into
DefectDojo:bugfixfrom
valentijnscholten:defender-stop-caching
May 22, 2025
Merged

ms defender: do not cache parsed findings#12493
valentijnscholten merged 2 commits into
DefectDojo:bugfixfrom
valentijnscholten:defender-stop-caching

Conversation

@valentijnscholten

@valentijnscholten valentijnscholten commented May 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

The MS Defender parser was caching parsed findings over time. This PR fixes that and adds a note to the how-to-write-a-parser guide.

Reported on community Slack: https://owasp.slack.com/archives/C2P5BA8MN/p1747924116271049?thread_ts=1747917376.987169&cid=C2P5BA8MN

@valentijnscholten valentijnscholten added this to the 2.46.4 milestone May 22, 2025
@dryrunsecurity

dryrunsecurity Bot commented May 22, 2025 •
edited
Loading

Copy link
Copy Markdown

DryRun Security

This pull request addresses multiple potential security and reliability issues across different parsers, including risks of information disclosure through logging, unhandled exceptions in vulnerability processing, and potential state persistence problems that could lead to incorrect parsing results.

💭 Unconfirmed Findings (6)
Vulnerability Potential Data Leakage in XML Parsing
Description Refactoring in the Fortify FPR parser could expose parsing details if the FortifyRelatedData class is not carefully implemented, potentially revealing sensitive parsing information.
Vulnerability Logging of Sensitive Parsing Details
Description Logs in the Fortify FPR parser might expose sensitive vulnerability information like instance IDs, with increased risk if log levels are not properly configured.
Vulnerability Potential Logging Information Disclosure
Description Debug logging statements in the MS Defender parser could reveal sensitive file names and parsing process details, posing a risk of information exposure in production environments.
Vulnerability Unhandled Exception in Vulnerability Processing
Description Exceptions during vulnerability processing in the MS Defender parser might silently drop vulnerabilities, with logging occurring but findings potentially not being added to the list.
Vulnerability Potential State Persistence Risk
Description Explicit reset of self.cvss_type in the PTART retest parser prevents unintended state persistence that could have caused subtle bugs with incorrect state carryover.
Vulnerability Potential Parser State Caching
Description Unit tests address the risk of findings being cached between parsing runs in the MS Defender parser, which could lead to incorrect or stale results if parser state is maintained between calls.

All finding details can be found in the DryRun Security Dashboard.

@valentijnscholten

valentijnscholten commented May 22, 2025

Copy link
Copy Markdown
Member Author

Can you also review @manuel-sommer?

@Maffooch Maffooch requested review from dogboat and hblankenship May 22, 2025 16:58
@valentijnscholten

valentijnscholten commented May 22, 2025

Copy link
Copy Markdown
Member Author

Found two other parsers that need a small update. I don't those are badly affected, but still good to not leave data lingering across scans. @dogboat @Maffooch

@Maffooch

Maffooch commented May 22, 2025

Copy link
Copy Markdown
Contributor

In the fortify parser, wouldn't all of this data be kept in memory until the next reset?
https://github.com/DefectDojo/django-DefectDojo/blob/5cd9876d3fda77746d3509d208af4a8454ab75d3/dojo/tools/fortify/fpr_parser.py#L15C5-L21C58

@valentijnscholten

valentijnscholten commented May 22, 2025

Copy link
Copy Markdown
Member Author

Yeah that's why I just pushed a commit for that :-)

@Maffooch

Maffooch commented May 22, 2025

Copy link
Copy Markdown
Contributor

Oh I see what you did now. I misread that the first time

mtesauro
mtesauro approved these changes May 22, 2025
View reviewed changes

@mtesauro mtesauro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@valentijnscholten valentijnscholten merged commit 4e3c6f4 into DefectDojo:bugfix May 22, 2025
78 checks passed
xansec pushed a commit to xansec/django-DefectDojo that referenced this pull request Jun 18, 2025
@valentijnscholten @xansec
ms defender: do not cache parsed findings (DefectDojo#12493)
f59f721 
* ms defender: do not cache parsed findings

* update other parsers class variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mtesauro mtesauro mtesauro approved these changes

@Maffooch Maffooch Maffooch approved these changes

@dogboat dogboat Awaiting requested review from dogboat

+1 more reviewer

@hblankenship hblankenship hblankenship approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs parser unittests

Projects

None yet

Milestone

2.46.4

Development

Successfully merging this pull request may close these issues.

5 participants

@valentijnscholten @Maffooch @mtesauro @dogboat @hblankenship